Improve locking #1
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The global lock just provides too much of a surface for contention and deadlocks. Global locking is still needed by the Xlib backend, since Xlib is not thread-safe and we're not enabling the parts that make it slightly more thread-safe, so it now has its own lock object. The docs were also updated to spell out our multithreading guarantees.
There was a problem hiding this comment.
Pull request overview
This PR refactors the locking mechanism in MSS to improve multithreading support and reduce contention. The global lock that was previously used for all screenshot operations has been replaced with per-object locks, allowing multiple MSS objects to be used concurrently from different threads and enabling a single MSS object to be safely shared across threads.
Key changes:
- Replaced global lock with per-object locks in the base class, improving concurrency
- Converted Windows backend from thread-local storage to instance variables protected by the per-object lock
- Introduced a separate global lock for the Xlib backend (which requires it due to Xlib's lack of thread-safety)
- Added comprehensive documentation about multithreading guarantees and constraints
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/mss/base.py | Added per-object _lock attribute, deprecated global lock, updated all methods to use per-object lock instead of global lock |
| src/mss/windows.py | Removed thread-local storage (local()), converted _handles to individual instance variables (_bmp, _bmi, _data, etc.) protected by per-object lock |
| src/mss/linux/xlib.py | Added module-level _lock for Xlib thread-safety, wrapped all Xlib calls with this lock while respecting lock ordering (per-object lock first, then global Xlib lock) |
| src/tests/test_windows.py | Updated tests to access instance variables directly (_bmp) instead of through thread-local storage (_handles.bmp) |
| docs/source/usage.rst | Added "Multithreading" section documenting thread-safety guarantees, constraints on custom ScreenShot classes, and warnings about Xlib backend limitations |
| docs/source/examples.rst | Added anchor reference for custom_cls_image to support cross-referencing from usage documentation |
| CHANGELOG.md | Added entry describing the multithreading improvements |
Review summary: I conducted a thorough review of the locking implementation, focusing on potential deadlocks, race conditions, and lock ordering issues. The implementation is well-designed and correctly handles:
- Lock ordering: The Xlib backend properly enforces that the per-object lock is acquired before the global Xlib lock, preventing deadlocks
- Thread safety: All shared mutable state is protected by appropriate locks
- Documentation: Clear warnings about the limitations of the Xlib backend and constraints on custom ScreenShot classes
- Test coverage: Existing thread safety tests validate the new implementation
No issues were found. The locking design is sound and the implementation follows best practices for concurrent programming.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I asked GitHub Copilot to review my changes. It didn't see any problems, but I looked at its thought process and cleaned up things that it had to think carefully about. * Fix typo in an older CHANGELOG entry. * Add comments about why each documented public attribute of MSSBase, and a few undocumented but not _-prefixed attributes of MSS implementations, are safe to not be protected under the lock. * Document that the user isn't supposed to change with_cursor. Also document that MSS might ignore it. * Clarify that the comment "The attributes below are protected by self._lock" is meant to contrast with the attributes above that comment. * In MSSBase.monitors, read the return value self._monitors under the lock. Current code doesn't ever change self._monitors once it's created, so it's safe to return outside the lock, but verifying that requires inspection of the rest of the codebase. This makes it clear and future-proof. * Add a test that we can use the same MSS object on multiple threads.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The global lock just provides too much of a surface for contention and
deadlocks. Global locking is still needed by the Xlib backend, since
Xlib is not thread-safe and we're not enabling the parts that make it
slightly more thread-safe, so it now has its own lock object.
The docs were also updated to spell out our multithreading guarantees.
This PR is not actually going to be merged into jholveck/python-mss:main. It exists to give GitHub Copilot a place to review the changes before we send them upstream to BoboTiG/python-mss (currently pending as BoboTiG#452 )
Locking is very tricky to get right, and it's easy to miss bugs. Copilot, I would appreciate it if you could take a look over this and see if I've overlooked anything. Thank you!